feat: optimize CASE WHEN for divide-by-zero protection pattern#19994
Draft
CuteChuanChuan wants to merge 6 commits intoapache:mainfrom
Draft
feat: optimize CASE WHEN for divide-by-zero protection pattern#19994CuteChuanChuan wants to merge 6 commits intoapache:mainfrom
CuteChuanChuan wants to merge 6 commits intoapache:mainfrom
Conversation
CuteChuanChuan
force-pushed
the
raymond/11570-optimize-case-when
branch
2 times, most recently
from
267d8df to
334785f
Compare
Contributor
|
run benchmark tpcds |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
Hi @andygrove , |
Contributor
|
run benchmark tpcds |
Contributor
|
Also FYI @pepijnve |
|
🤖 |
Contributor
|
It might be useful to add a microbenchmark to https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/benches/case_when.rs so we can compare before/after. I don't think any of the existing ones will cover this particular pattern. The TCP-DS queries that contain this pattern (based on |
|
🤖: Benchmark completed Details
|
- Adds a specialization for the common pattern: CASE WHEN y > 0 THEN x / y ELSE NULL END - Add EvalMethod::DivideByZeroProtection variant - Add pattern detection in find_best_eval_method() - Implement safe_divide using Arrow kernels - Handle CastExpr wrapping on divisor
CuteChuanChuan
force-pushed
the
raymond/11570-optimize-case-when
branch
from
334785f to
6479884
Compare
Contributor
Author
|
Microbenchmark result:
|
Contributor
Author
|
Hi @pepijnve , thanks for pointing out the need for a microbenchmark. I compared |
alamb
reviewed
Jan 29, 2026
| } | ||
| } | ||
|
|
||
| fn benchmark_divide_by_zero_protection(c: &mut Criterion, batch_size: usize) { |
Contributor
There was a problem hiding this comment.
Could you please pull this additional benchmark code code into its own PR we could then use our benchmarking scripts to compare performance of this PR with main?
Thank you 🙏
Contributor
Author
pepijnve
reviewed
Jan 30, 2026
| }, | ||
| ); | ||
|
|
||
| group.bench_function( |
Contributor
There was a problem hiding this comment.
I'm trying to spot the difference between this benchmark and the one just above it, they look identical to me. Yet the measurements seem to produce similar values. What causes that?
Contributor
Author
There was a problem hiding this comment.
Hi @pepijnve ,
The key difference is in which column is used in the WHEN condition:
- First benchmark (
DivideByZeroProtection): checks divisor_col > 0, and since divisor_col is also the divisor in numerator / divisor_col, this matches the pattern and triggers the optimization. - Second benchmark (
ExpressionOrExpression): checks divisor_copy_col > 0, but the division uses divisor_col. Since the checked column doesn't match the divisor, the optimization is not triggered and it falls back to ExpressionOrExpression.
I'll also add a comment in the code to make this distinction clearer.
Contributor
There was a problem hiding this comment.
Ah of course. That's really subtle. A comment for future readers is indeed a good idea.
Contributor
There was a problem hiding this comment.
A followup question I had, but I'm happy to measure that myself, is what's causing the performance gap? Might be interesting to see if there's something we could do to improve ExprOrExpr for this particular pattern.
Contributor
Author
There was a problem hiding this comment.
Hi @pepijnve,
I think the performance gap might come from execution model differences. Please correct me if I am wrong.
ExprOrExpr: evaluate condition → build selection → execute on selected rows → merge
DivideByZeroProtection: fully vectorized Arrow kernels
The vectorized path avoids selection/branching overhead, which is why it wins when most rows need computation (low zero density). However, ExprOrExpr's short-circuit helps when many rows are filtered (high zero density).
Although I don't have concrete ideas about potential ExprOrExpr improvements for this pattern yet, I'd be happy to explore further if you have any suggestions!
Contributor
There was a problem hiding this comment.
I've been studying the results a bit. I'll add some extra comments on the relevant bits of code.
Contributor
There was a problem hiding this comment.
There is another PR here from @pepijnve to improve some cases
Contributor
|
Benchmark was added in #20076 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
## Which issue does this PR close? Related to #19994 - This PR extracts the benchmark code to allow performance comparison. ## Rationale for this change As pointed out by @alamb in #19994, this separates the microbenchmark code so that the benchmarking scripts can compare the optimization PR against main with the benchmark already in place. ## What changes are included in this PR? Adds a microbenchmark for the divide-by-zero protection pattern in `case_when.rs`: - Benchmarks with varying percentages of zeros (0%, 10%, 50%, 90%) - Compares `DivideByZeroProtection` pattern (where checked column matches divisor) vs `ExpressionOrExpression` fallback (where they don't match) ## Are these changes tested? benchmark code only. ## Are there any user-facing changes? No.
Contributor
|
I merged up to get the new benchmarks so I could run them on our runner |
Contributor
|
run benchmark case_when |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
run benchmark case_when |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
Contributor
Author
@pepijnve |
Contributor
|
run benchmark case_when |
|
🤖 |
Contributor
|
running with updated benchmarks |
Contributor
|
run benchmark case_when |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
Divide by zero protection is looking pretty good: For some reason the CI is now failing on this PR When #20097 is merged, then we can run the benchmarks again and see how much additional boost this special case gives us |
Contributor
That's due to #19994 (comment) |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 3, 2026
…0097) ## Which issue does this PR close? - Related to #11570 ## Rationale for this change While reviewing #19994 it became clear the optimised `ExpressionOrExpression` code path was not being used when the case expression has no `else` branch or has `else null`. In those situations the general evaluation strategies could end up being used. This PR refines the `ExpressionOrExpression` implementation to also handle `else null` expressions. ## What changes are included in this PR? Use `ExpressionOrExpression` for expressions of the form `CASE WHEN x THEN y [ELSE NULL]` ## Are these changes tested? Covered by existing SLTs ## Are there any user-facing changes? No
CuteChuanChuan
force-pushed
the
raymond/11570-optimize-case-when
branch
from
4319634 to
0a8f085
Compare
Contributor
|
run benchmark case_when |
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
It seems that effect of Divide by zero protection is not significant.🤣 |
Contributor
Those last results are in line with what I was seeing locally. You get a speedup when there is a small percentage of zeroes, but a slow down when there are many. Not sure what the wisest choice is for this one. Personally I think I’m inclined to keep focussing on squeezing more out of the general pattern code paths than adding very specialized ones. Perhaps with some more profiling and optimisation work we can shave some more off the general 10% variant for instance? I haven’t looked at that closely yet. |
Contributor
Contributor
Author
Closing this PR and pursuing more general optimization seems more reasonable. Thanks @alamb and @pepijnve for very informative feedback and discussion.❤️ |
Contributor
|
@CuteChuanChuan just wanted to second the positive sentiment expressed by @alamb. Taking a closer look at this with the profiler showed that my assumptions wrt which code path was being taken were wrong and that there was still some low hanging fruit left in the general code paths. I've left some pointers in #11570 (comment) regarding possible future work. We might be able to squeeze some better performance out of |
Contributor
|
Thanks again @pepijnve and @CuteChuanChuan |
Contributor
Author
I'd like to give it a try. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The
CaseExprimplementation is expensive. A common usage pattern (particularly in TPC-DS benchmarks) is to protect against divide-by-zero:This entire expression can be replaced with a simpler divide operation that returns NULL when the divisor is zero, avoiding the overhead of full CASE evaluation.
What changes are included in this PR?
Are these changes tested?
Yes, added two new tests:
Are there any user-facing changes?
No. This is an internal optimization that produces the same results but with better performance.